Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow c_data() to return zero byte buffer #2654

Merged
merged 3 commits into from
Jun 18, 2023

Conversation

kevinbackhouse
Copy link
Collaborator

@kevinbackhouse kevinbackhouse commented Jun 17, 2023

This reverts a change that was made in #2209. I argued at the time that it was a bad idea. Since then it has caused at least two bugs that I'm aware of: #2565, #2650.

@ghost
Copy link

ghost commented Jun 17, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@kmilos
Copy link
Collaborator

kmilos commented Jun 17, 2023

It'll still return an address not owned by the DataBuf, no?

@kevinbackhouse
Copy link
Collaborator Author

It'll still return an address not owned by the DataBuf, no?

It's the top edge of the buffer, so it's fine as long as you read zero bytes. That's quite a common edge case, so I think it should be allowed.

@codecov
Copy link

codecov bot commented Jun 17, 2023

Codecov Report

Merging #2654 (5f9ecad) into main (b66ebd9) will decrease coverage by 0.01%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #2654      +/-   ##
==========================================
- Coverage   63.92%   63.92%   -0.01%     
==========================================
  Files         103      103              
  Lines       22306    22306              
  Branches    10795    10795              
==========================================
- Hits        14260    14259       -1     
  Misses       5826     5826              
- Partials     2220     2221       +1     
Impacted Files Coverage Δ
src/types.cpp 95.53% <50.00%> (-0.28%) ⬇️

@kmilos
Copy link
Collaborator

kmilos commented Jun 17, 2023

Isn't it a potential exploit? That we would provide on our public API?

@kevinbackhouse
Copy link
Collaborator Author

Isn't it a potential exploit? That we would provide on our public API?

I don't think so. The old code only protected against out-of-bounds reads that looked like this:

memcpy(mybuf, buf.c_data(buf.size()), 10);

But it didn't protect against this, which is almost exactly the same thing:

memcpy(mybuf, buf.c_data(buf.size() - 1), 11);

@kevinbackhouse
Copy link
Collaborator Author

That fuzzer failure is concerning. It probably means that this code was masking a bug somewhere else. I'll investigate.

@kmilos
Copy link
Collaborator

kmilos commented Jun 17, 2023

I wasn't thinking about our code.

@kevinbackhouse
Copy link
Collaborator Author

I'm sorry, I made a mistake. It turns out there's a similar assertion in std::vector::operator[], which is what's causing the fuzzing failure. I've added a third commit which returns nullptr in this edge case. I think that will work well because it stops this code from throwing an exception in the edge case where the client is reading zero bytes at the end of the buffer, but it will throw a null pointer exception if they actually try to read the out-of-bounds bytes.

@neheb
Copy link
Collaborator

neheb commented Jun 17, 2023

@caclark this fix things?

@caclark
Copy link

caclark commented Jun 17, 2023

@neheb No, unfortunately this does not fix the Geeqie related bug.

@neheb neheb merged commit 3202e86 into Exiv2:main Jun 18, 2023
@neheb
Copy link
Collaborator

neheb commented Jun 18, 2023

@Mergifyio backport 0.28.x

@mergify
Copy link
Contributor

mergify bot commented Jun 18, 2023

backport 0.28.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants